Bugfix: RGB/Hue Curve Performance with MSL - #2342
Conversation
The MSL class wrapper gave the generated struct owning copies of every array uniform, with a constructor that copied the used range out of constant memory into per-thread private memory and then zero-filled the remainder up to the full declared capacity. The struct is instantiated once per shader invocation, so a compute shader dispatched one thread per pixel paid that copy per pixel: for a dynamic GradingRGBCurve that is 8 + 120 + 8 + 360 = 496 private-memory writes for every pixel, regardless of how many knots the curve actually has. Point at the constant memory instead. The array declarations arrive from the ops shared with the other GPU languages, so the wrapper rewrites them to constant pointers rather than changing the ops; it already knows which parameters are arrays. Indexing syntax is unchanged, so every read site in the generated shader is untouched, and the generated function signature is unchanged so existing MSL consumers keep working. Dropping the zero-fill does not change any result. Reads of knots/coefs are bounded by the offset taken from the offsets array, and the offsets arrays are always fully populated -- GetNumOffsetValues() is a compile-time constant equal to the declared capacity (8 for RGB, 16 for hue) -- so no read past the used range was ever possible. Measured with a Metal compute kernel over an RGBA16F 4K UHD frame, one thread per pixel, output texels bit-identical before and after: rgb curve only 33.45 ms -> at the read+write bandwidth floor luma curve only 35.65 ms -> at the floor hue curve only 35.46 ms -> at the floor rgb + luma + hue 115.28 ms -> at the floor GradingRGBCurve and GradingHueCurve are the only ops that declare array uniforms; everything else passes scalars or textures and is unaffected. MetalSupport9 pins the generated text, so its expected shader is updated. Three assertions are added alongside it stating the no-copy invariant on its own, so a future refresh of the expected text cannot quietly restore the per-pixel copy. Signed-off-by: Carol Payne <[email protected]>
carolalynn
added
the
Bug
carolalynn
changed the title
doug-walker
left a comment
doug-walker
left a comment
There was a problem hiding this comment.
Looks like a nice improvement!
| // fixed-size members i.e. 'float name[120];'. Owning them would make the constructor copy the | ||
| // whole array out of constant memory into per-thread memory, which is prohibitive as the | ||
| // struct is typically instantiated once per pixel. Hold a pointer to the constant memory | ||
| // instead; every read site is unchanged as the indexing syntax is the same. |
There was a problem hiding this comment.
Is it true that "the struct is typically instantiated once per pixel"? Isn't it more typical that a group of pixels is processed in a batch, to amortize cost like this?
I'm all in favor of the change to improve performance for the single-pixel case, but I suspect it's not the "typical" situation.
There was a problem hiding this comment.
Fair — "typically" was sloppy. It's not really a claim about how you dispatch: the struct is a temporary constructed inside the generated OCIOMain(), which takes and returns one float4. No batch entry point, nowhere to hoist it to, so it's once per call.
The numbers are from the normal case rather than a synthetic one — the 30.01 → 0.45 ms row is a fragment shader built the way metalapp.mm generates one, and compute (29.30 → 0.44) lands in the same place, so nothing is amortizing it either way.
You're right that a kernel doing a tile per thread could hoist it. That's just not what the wrapper emits today, and even there the copy is still per-thread and a 496-float private array costs occupancy anyway.
Reworded the comment to give the mechanism instead.
| OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots = " | ||
| "ocio_grading_rgbcurve_knots;") != std::string::npos); | ||
| OCIO_CHECK_ASSERT(text.find("this->ocio_grading_rgbcurve_knots[i]") | ||
| == std::string::npos); |
There was a problem hiding this comment.
Maybe this is a nit pick but it seems like kind of an odd test. The first two asserts are redundant after the comparison of the shader program above. And for the third, if we don't want people to change the code back in the future, perhaps just a comment where the original code is should suffice? If someone did try to return to the original approach, it would have to be written exactly the same in order to fail this test (e.g., maybe someone uses j rather than i for their loop)?
There was a problem hiding this comment.
Agreed, dropped all three.
They were aimed at someone regenerating expected wholesale from actual output, where the equality check passes and a restored copy slips through. But the negative one only catches a character-identical reintroduction, so it doesn't really do that job — and the first two are redundant, as you say.
The explanation lives in the comment on rewriteArrayDeclarations now.
…ext asserts Signed-off-by: Carol Payne <[email protected]>
2 participants
While implementing RGB and Hue Curves, noticed a severe performance drop in playback on edited curves vs. other OCIO ops. This fix is specific to Metal on MacOS, so I have already asked @Morteeza to take a look on my fork - fix confirmed.
The MSL class wrapper gave the generated struct owning copies of every array uniform, with a constructor that copied the used range out of constant memory into per-thread private memory and then zero-filled the remainder up to the full declared capacity. The struct is instantiated once per shader invocation, so a compute shader dispatched one thread per pixel paid that copy per pixel: for a dynamic GradingRGBCurve that is 8 + 120 + 8 + 360 = 496 private-memory writes for every pixel, regardless of how many knots the curve actually has.
Point at the constant memory instead. The array declarations arrive from the ops shared with the other GPU languages, so the wrapper rewrites them to constant pointers rather than changing the ops; it already knows which parameters are arrays. Indexing syntax is unchanged, so every read site in the generated shader is untouched, and the generated function signature is unchanged so existing MSL consumers keep working.
Dropping the zero-fill does not change any result. Reads of knots/coefs are bounded by the offset taken from the offsets array, and the offsets arrays are always fully populated -- GetNumOffsetValues() is a compile-time constant equal to the declared capacity (8 for RGB, 16 for hue) -- so no read past the used range was ever possible.
Measured with a Metal compute kernel over an RGBA16F 4K UHD frame, one thread per pixel, output texels bit-identical before and after:
rgb curve only 33.45 ms -> at the read+write bandwidth floor
luma curve only 35.65 ms -> at the floor
hue curve only 35.46 ms -> at the floor
rgb + luma + hue 115.28 ms -> at the floor
GradingRGBCurve and GradingHueCurve are the only ops that declare array uniforms; everything else passes scalars or textures and is unaffected.
MetalSupport9 pins the generated text, so its expected shader is updated. Three assertions are added alongside it stating the no-copy invariant on its own, so a future refresh of the expected text cannot quietly restore the per-pixel copy.
Assisted by: Claude / Opus 5